Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[podmanreceiver] add scraper's shutdown method #32981

Merged
merged 18 commits into from
May 31, 2024
Merged

[podmanreceiver] add scraper's shutdown method #32981

merged 18 commits into from
May 31, 2024

Conversation

rogercoll
Copy link
Contributor

Description:

Link to tracking Issue: Related issue #29994

Testing:
Use scraper instead of metrics receiver interface.

Documentation:

@rogercoll rogercoll requested review from a team and atoulme May 10, 2024 14:04
@rogercoll
Copy link
Contributor Author

No changes on the end user, skip changelog?

crobert-1
crobert-1 reviewed May 10, 2024
View reviewed changes
Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since shutdown wasn't being called properly before (and we were leaking a goroutine), this can be considered a bug fix, so we probably want a changelog.

receiver/podmanreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/podmanreceiver/receiver_test.go Outdated Show resolved Hide resolved
@crobert-1
Copy link
Member

CI/CD failure:

Error: ../../receiver/podmanreceiver/factory.go:54:10: assignment mismatch: 1 variable but newMetricsReceiver returns 2 values
Error: ../../receiver/podmanreceiver/factory.go:54:51: not enough arguments in call to newMetricsReceiver
	have (receiver.CreateSettings, *Config, nil)
	want (context.Context, receiver.CreateSettings, *Config, consumer.Metrics, any)
make: *** [Makefile:[32](https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/9033747495/job/24824836747?pr=32981#step:7:33)7: otelcontribcol] Error 1

@rogercoll rogercoll force-pushed the add_shutdown_podman branch from 254d18d to 2335536 Compare May 11, 2024 15:04
@rogercoll rogercoll force-pushed the add_shutdown_podman branch from 2335536 to 9a82696 Compare May 11, 2024 15:28
crobert-1
crobert-1 approved these changes May 13, 2024
View reviewed changes
Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing!

.chloggen/add_shutdown_podman.yaml Outdated Show resolved Hide resolved
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented May 19, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@atoulme
Copy link
Contributor

atoulme commented May 19, 2024

See CLA issues.

@rogercoll
Copy link
Contributor Author

rogercoll commented May 20, 2024 •
edited
Loading

Seems to be related with the suggested applied commits. Could you take a look @crobert-1 ?

@crobert-1
Copy link
Member

/easycla

@crobert-1
Copy link
Member

My apologies, not sure what's going on here. I've filed a support ticket against EasyCLA, I'll share any relevant updates.

@crobert-1
Copy link
Member

Ticket update:

On Friday we released co-author checks in EasyCLA. It seems there is a bug win the logic that is causing the missing ID on commit scenario. I am engaging our engineers to investigate this further. Thank you for reporting this issue.

@jarias-lfx jarias-lfx mentioned this pull request May 20, 2024
Open
17 tasks
@crobert-1 crobert-1 mentioned this pull request May 21, 2024
Merged
@jarias-lfx
Copy link

/easycla

andrzej-stencel
andrzej-stencel reviewed May 30, 2024
View reviewed changes
receiver/podmanreceiver/receiver_windows.go
) (receiver.Metrics, error) {
podmanConfig := config.(*Config)

return newMetricsReceiver(params, podmanConfig, nil, consumer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why bother to have the newMetricsReceiver function on Windows? Why not return the error directly from createMetricsReceiver?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, I was thinking to even remove the windows file as the docker receiver implementation.

andrzej-stencel
andrzej-stencel reviewed May 31, 2024
View reviewed changes
receiver/podmanreceiver/receiver.go
}

func (r *metricsReceiver) start(ctx context.Context, _ component.Host) error {
var err error
err := r.config.Validate()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you calling Config.Validate in the component start function? I don't think this function should be called by component authors. It is called for each component during collector setup:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I will open a PR to remove it

@andrzej-stencel andrzej-stencel merged commit e13b1a3 into open-telemetry:main May 31, 2024
162 checks passed
@github-actions github-actions bot added this to the next release milestone May 31, 2024
@rogercoll rogercoll deleted the add_shutdown_podman branch June 10, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crobert-1 crobert-1 crobert-1 approved these changes

@andrzej-stencel andrzej-stencel andrzej-stencel left review comments

@atoulme atoulme Awaiting requested review from atoulme

Assignees

@MovieStoreGuy MovieStoreGuy

Labels
receiver/podman
Projects
None yet
Milestone
v0.102.0
Development

Successfully merging this pull request may close these issues.
6 participants
@rogercoll @crobert-1 @atoulme @jarias-lfx @andrzej-stencel @MovieStoreGuy